feat(flags): Add per-team rate limiting to flag definitions endpoint #39923
Conversation
haacked
force-pushed
the
haacked/flag-definitions-throttling
branch
from
d1b1ff0 to
4eefc05
Compare
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
haacked
force-pushed
the
haacked/flag-definitions-throttling
branch
2 times, most recently
from
1d937cd to
9394c50
Compare
gustavohstrassburger
left a comment
gustavohstrassburger
left a comment
There was a problem hiding this comment.
Solid, nice work on the tests too.
dmarticus
left a comment
dmarticus
left a comment
There was a problem hiding this comment.
non-blocking, but I did have a comment asking why we're rolling our own rate limiter vs using an existing one
| } | ||
|
|
||
| /// Type alias for flag definitions rate limiting (per-team) | ||
| pub type FlagDefinitionsRateLimiter = KeyedRateLimiter<TeamId>; |
There was a problem hiding this comment.
I like this, and the implementation is solid, but is there a reason why you shipped your own rate limited instead of using, say, tower_governor? We definitely have more control over how we do rate limiting by shipping our own but it may mean that as we go, we'll need to keep contributing to this tool instead of using one that supports the features we need natively. Given that we're already importing governor, it seems like a natural extension to use this lib too.
There was a problem hiding this comment.
I just didn't know about it. Let me look into it.
There was a problem hiding this comment.
After looking into tower-governor, I'm not sure it meets our needs unless we want to change how we're doing rate limiting. These are the things it doesn't support:
- Dynamic, per-team quotas (it uses a single static config per layer)
- Post-auth keying (our limiter runs after the team is identified)
- Runtime switching of key extractors (open issue upstream)
Our implementation is basically a thin wrapper around the same underlying governor crate, so we get similar semantics while keeping flexibility for per-team limits and metrics integration. If tower-governor adds per-key quota support later, migrating should be straightforward.
haacked
force-pushed
the
haacked/flag-definitions-throttling
branch
from
9394c50 to
eb4eb65
Compare
FlagDefinitionsRateLimiterImplements configurable rate limiting for the `/flags/definitions` endpoint to protect against excessive requests and allow per-team customization.
Key features:
- Configurable default rate limit via `FLAG_DEFINITIONS_DEFAULT_RATE_PER_MINUTE` (default: 600/minute)
- Per-team overrides via `FLAG_DEFINITIONS_RATE_LIMITS` environment variable Format: `{"team_id": "rate_string"}` (e.g., `{"123": "1200/minute", "456": "2400/hour"}`)
- Supports Django `SimpleRateThrottle` rate format (N/second|minute|hour|day)
- Rate limiting occurs after authentication to prevent enumeration attacks
- Thread-safe implementation using governor with `Arc<RwLock>`
- Prometheus metrics for monitoring:
- `flags_flag_definitions_requests_total`
- `flags_flag_definitions_rate_limited_total`
Implementation:
- Generic `KeyedRateLimiter<K>` struct for reusable rate limiting with any key type
- Configurable Prometheus metrics via constructor parameters
- Uses GCRA (Generic Cell Rate Algorithm) via `governor` crate for efficiency
- `rate_parser` module for parsing Django-style rate strings
- Renamed `local_evaluation` module to `flag_definitions` for clarity
- Integrated into `flags_definitions` handler in `flag_definitions` module
- Comprehensive test coverage (9 unit tests, 24 integration tests)
Module refactoring:
- local_evaluation.rs → flag_definitions.rs
- test_local_evaluation.rs → test_flag_definitions.rs
- LocalEvaluationResponse → FlagDefinitionsResponse
- LocalEvaluationQueryParams → FlagDefinitionsQueryParams
- authenticate_local_evaluation → authenticate_flag_definitions
- FlagRequestType::LocalEvaluation → FlagRequestType::FlagDefinitions
Environment variables:
- `FLAG_DEFINITIONS_DEFAULT_RATE_PER_MINUTE`: Default rate for all teams (default: 600)
- `FLAG_DEFINITIONS_RATE_LIMITS`: JSON map of team_id to rate string for overrides
haacked
force-pushed
the
haacked/flag-definitions-throttling
branch
from
eb4eb65 to
baf1a9f
Compare
Problem
The new Rust
/flags/definitionsendpoint needs rate limiting the same way/local_evaluationdoes.Changes
Implements configurable rate limiting for the
/flags/definitionsendpoint to protect against excessive requests and allow per-team customization.Key features:
FLAG_DEFINITIONS_DEFAULT_RATE_PER_MINUTE(default: 600/minute)FLAG_DEFINITIONS_RATE_LIMITSenvironment variable Format:{"team_id": "rate_string"}(e.g.,{"123": "1200/minute", "456": "2400/hour"})SimpleRateThrottlerate format (N/second|minute|hour|day)Arc<RwLock>flags_flag_definitions_requests_totalflags_flag_definitions_rate_limited_totalImplementation:
KeyedRateLimiter<K>struct for reusable rate limiting with any key typegovernorcrate for efficiencyrate_parsermodule for parsing Django-style rate stringslocal_evaluationmodule toflag_definitionsfor clarityflags_definitionshandler inflag_definitionsmoduleModule refactoring:
Environment variables:
FLAG_DEFINITIONS_DEFAULT_RATE_PER_MINUTE: Default rate for all teams (default: 600)FLAG_DEFINITIONS_RATE_LIMITS: JSON map of team_id to rate string for overridesHow did you test this code?
Manually
Unit Tests
👉 Stay up-to-date with PostHog coding conventions for a smoother review.
Changelog: (features only) Is this feature complete?